feat(vision): make sidecar reasoning configurable - #1002
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis PR adds configurable vision sidecar reasoning levels. The setting flows through configuration validation, provider requests, cache keys, management APIs, dashboard controls, tests, translations, and localized documentation. ChangesVision Reasoning Effort Implementation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant ConfigAPI
participant VisionIndex
participant DescribeImage
participant VisionBackend
Dashboard->>ConfigAPI: PUT visionSidecar.reasoning
ConfigAPI->>ConfigAPI: validate and persist reasoning
ConfigAPI-->>Dashboard: return persisted reasoning
VisionIndex->>VisionIndex: build plan and cache identity
VisionIndex->>DescribeImage: pass reasoning settings
DescribeImage->>VisionBackend: send vision request with reasoning
VisionBackend-->>DescribeImage: return image description
DescribeImage-->>VisionIndex: return description
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1122fe5cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| settings: { model: resolveOpenAiVisionModel(config), timeoutMs: cfg.timeoutMs ?? DEFAULT_TIMEOUT_MS }, | ||
| settings: { | ||
| model: resolveOpenAiVisionModel(config), | ||
| reasoning: cfg.reasoning ?? DEFAULT_REASONING, |
There was a problem hiding this comment.
When visionSidecar.reasoning is written through ocx config set/import or by editing config.json, configSchema passes the sidecar object through and validateConfigCandidate() therefore accepts arbitrary values such as "ultra"; this line then forwards that value as reasoning.effort, causing every OpenAI vision description to be rejected upstream even though the management endpoint rejects the same input. Add this enum to the persisted-config validation/normalization path, or normalize it before constructing VisionSettings.
Useful? React with 👍 / 👎.
| key: JSON.stringify([ | ||
| plan.backend, | ||
| plan.settings.model, | ||
| plan.settings.reasoning, |
There was a problem hiding this comment.
When the selected vision backend is Anthropic and the operator changes this dashboard-exposed reasoning setting, the Anthropic request remains identical (anthropic-describe.ts always sends thinking: { type: "disabled" } and never reads settings.reasoning), but this key changes. Consequently the same image and context bypass an otherwise valid cached description and trigger another potentially billed OAuth sidecar call; include reasoning in the identity only for the OpenAI backend, or key on the effective Anthropic settings.
Useful? React with 👍 / 👎.
| /> | ||
| <Select | ||
| value={sidecar?.vision.reasoning ?? "low"} | ||
| options={VISION_REASONING_LEVELS.map(value => ({ value, label: value }))} |
There was a problem hiding this comment.
This picker offers max for every OpenAI vision model, but the repository's native catalog deliberately gives only GPT-5.6 models the max rung and limits native gpt-5.5, gpt-5.4, and gpt-5.4-mini to low through xhigh (src/codex/catalog/sync.ts). Selecting max with one of those models therefore persists a value that passes the management API's universal enum check and is sent raw as reasoning.effort, causing the vision request to fail; derive the choices from the selected model's reasoningEfforts metadata or validate/clamp the pair server-side.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/guides/sidecars.md`:
- Around line 79-81: Rewrite the sidecar image-processing descriptions to
distinguish provider-specific transports: in
docs-site/src/content/docs/guides/sidecars.md lines 79-81, explain ChatGPT-login
passthrough uses the OpenAI Responses endpoint and that Anthropic uses the
Messages path with its thinking-budget mapping; apply the equivalent provider
split in Japanese at docs-site/src/content/docs/ja/guides/sidecars.md lines
78-80, Korean at docs-site/src/content/docs/ko/guides/sidecars.md lines 79-81,
Russian at docs-site/src/content/docs/ru/guides/sidecars.md lines 90-92, and
Chinese at docs-site/src/content/docs/zh-cn/guides/sidecars.md lines 72-73,
preserving each language’s accuracy and alignment with the provider contracts.
In `@docs-site/src/content/docs/ja/reference/configuration/server.md`:
- Line 147: Update the reasoning configuration references to add one concise
upstream-support caveat in each affected table or adjacent paragraph:
docs-site/src/content/docs/ja/reference/configuration/server.md:147-147 in
Japanese,
docs-site/src/content/docs/ko/reference/configuration/server.md:147-147 in
Korean, docs-site/src/content/docs/reference/configuration/server.md:179-179 in
English, docs-site/src/content/docs/ru/reference/configuration/server.md:182-182
in Russian, and
docs-site/src/content/docs/zh-cn/reference/configuration/server.md:162-162 in
Chinese. State that supported reasoning levels are gated by upstream provider
capabilities and the provider’s available reasoning ladder.
In `@gui/src/i18n/ru.ts`:
- Line 217: Update the dash.visionReasoning translation to replace the English
“vision” with the file’s established Russian wording “для изображений,” matching
the phrasing pattern used by dash.searchReasoning and related vision keys.
In `@src/server/management/config-routes.ts`:
- Around line 347-352: Extract the shared vision reasoning allowlist into a
single constant in the config-routes module, then reuse it in both the
validation branch around the vision.reasoning check and the persistence branch
around the second repeated check. Replace both duplicated OR chains with
membership checks against that constant so both paths always accept the same
five values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 006a539f-e1bf-41f8-987e-3ffa15d8d192
📒 Files selected for processing (27)
docs-site/src/content/docs/guides/sidecars.mddocs-site/src/content/docs/ja/guides/sidecars.mddocs-site/src/content/docs/ja/reference/configuration/server.mddocs-site/src/content/docs/ko/guides/sidecars.mddocs-site/src/content/docs/ko/reference/configuration/server.mddocs-site/src/content/docs/reference/configuration/server.mddocs-site/src/content/docs/ru/guides/sidecars.mddocs-site/src/content/docs/ru/reference/configuration/server.mddocs-site/src/content/docs/zh-cn/guides/sidecars.mddocs-site/src/content/docs/zh-cn/reference/configuration/server.mdgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/pages/dashboard-overview-sections.tsxgui/src/pages/dashboard-shared.tsgui/tests/dashboard-contracts.test.tssrc/server/management/config-routes.tssrc/types.tssrc/vision/describe.tssrc/vision/index.tstests/claude-sidecar-override.test.tstests/vision-anthropic.test.tstests/vision-cache.test.tstests/vision-sidecar-e2e.test.ts
|
Review feedback addressed in 47e2540.
Validation: full server suite 8217 passed / 0 failed, GUI suite 585 passed / 0 failed, typecheck, GUI lint+build, docs build (216 pages), and privacy scan all green. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs-site/src/content/docs/guides/sidecars.md (1)
95-96: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the cache-key documentation across all locales.
The runtime includes reasoning in the description cache identity only for the OpenAI backend. Anthropic intentionally excludes the OpenAI-specific setting. Update each affected sentence to state this backend-specific behavior.
docs-site/src/content/docs/guides/sidecars.md#L95-L96: qualify reasoning as an OpenAI-only cache-key component.docs-site/src/content/docs/ja/guides/sidecars.md#L93-L94: qualify reasoning as an OpenAI-only cache-key component.docs-site/src/content/docs/ko/guides/sidecars.md#L93-L96: qualify reasoning as an OpenAI-only cache-key component.docs-site/src/content/docs/ru/guides/sidecars.md#L107-L108: qualify reasoning as an OpenAI-only cache-key component.docs-site/src/content/docs/zh-cn/guides/sidecars.md#L85-L87: qualify reasoning as an OpenAI-only cache-key component.docs-site/src/content/docs/reference/configuration/server.md#L186-L188: qualify reasoning as an OpenAI-only cache-key component.docs-site/src/content/docs/ja/reference/configuration/server.md#L151-L151: qualify reasoning as an OpenAI-only cache-key component.docs-site/src/content/docs/ko/reference/configuration/server.md#L151-L151: qualify reasoning as an OpenAI-only cache-key component.docs-site/src/content/docs/ru/reference/configuration/server.md#L189-L192: qualify reasoning as an OpenAI-only cache-key component.docs-site/src/content/docs/zh-cn/reference/configuration/server.md#L166-L166: qualify reasoning as an OpenAI-only cache-key component.As per path instructions, keep translated documentation synchronized with actual runtime behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs-site/src/content/docs/guides/sidecars.md` around lines 95 - 96, Update the cache-key sentences to state that reasoning is included only for the OpenAI backend, while preserving the existing exclusion for Anthropic. Apply this wording correction in docs-site/src/content/docs/guides/sidecars.md:95-96, docs-site/src/content/docs/ja/guides/sidecars.md:93-94, docs-site/src/content/docs/ko/guides/sidecars.md:93-96, docs-site/src/content/docs/ru/guides/sidecars.md:107-108, docs-site/src/content/docs/zh-cn/guides/sidecars.md:85-87, docs-site/src/content/docs/reference/configuration/server.md:186-188, docs-site/src/content/docs/ja/reference/configuration/server.md:151-151, docs-site/src/content/docs/ko/reference/configuration/server.md:151-151, docs-site/src/content/docs/ru/reference/configuration/server.md:189-192, and docs-site/src/content/docs/zh-cn/reference/configuration/server.md:166-166, keeping each translation synchronized with the runtime behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/guides/sidecars.md`:
- Around line 79-82: Add a caveat after each provider-specific transport
description explaining that the reasoning ladder support is constrained by the
selected provider and model, and that the Dashboard clamps persisted values when
a model does not advertise the selected reasoning level. This caveat must be
added at the following locations with equivalent translations or localized
versions: English version in docs-site/src/content/docs/guides/sidecars.md at
lines 79-82 (anchor), Japanese version in
docs-site/src/content/docs/ja/guides/sidecars.md at lines 78-81, Korean version
in docs-site/src/content/docs/ko/guides/sidecars.md at lines 79-82, Russian
version in docs-site/src/content/docs/ru/guides/sidecars.md at lines 90-93, and
Simplified Chinese version in
docs-site/src/content/docs/zh-cn/guides/sidecars.md at lines 72-74. Ensure the
caveat does not imply that advertised reasoning levels are universally supported
across all models or providers.
In `@src/config.ts`:
- Around line 1657-1662: Preserve an untouched copy of parsed before
sanitizeVisionSidecarForLoad mutates it, then pass that original data to both
warnDegradedVisionReasoning calls while continuing to use the sanitized parsed
object for schema parsing and other behavior. Update the relevant load paths
around sanitizeVisionSidecarForLoad and both warning-call sites without changing
the sanitizer’s existing validation behavior.
---
Outside diff comments:
In `@docs-site/src/content/docs/guides/sidecars.md`:
- Around line 95-96: Update the cache-key sentences to state that reasoning is
included only for the OpenAI backend, while preserving the existing exclusion
for Anthropic. Apply this wording correction in
docs-site/src/content/docs/guides/sidecars.md:95-96,
docs-site/src/content/docs/ja/guides/sidecars.md:93-94,
docs-site/src/content/docs/ko/guides/sidecars.md:93-96,
docs-site/src/content/docs/ru/guides/sidecars.md:107-108,
docs-site/src/content/docs/zh-cn/guides/sidecars.md:85-87,
docs-site/src/content/docs/reference/configuration/server.md:186-188,
docs-site/src/content/docs/ja/reference/configuration/server.md:151-151,
docs-site/src/content/docs/ko/reference/configuration/server.md:151-151,
docs-site/src/content/docs/ru/reference/configuration/server.md:189-192, and
docs-site/src/content/docs/zh-cn/reference/configuration/server.md:166-166,
keeping each translation synchronized with the runtime behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5a0ab5cb-72fb-40da-9e5d-9946acf3960d
📒 Files selected for processing (23)
docs-site/src/content/docs/guides/sidecars.mddocs-site/src/content/docs/ja/guides/sidecars.mddocs-site/src/content/docs/ja/reference/configuration/server.mddocs-site/src/content/docs/ko/guides/sidecars.mddocs-site/src/content/docs/ko/reference/configuration/server.mddocs-site/src/content/docs/reference/configuration/server.mddocs-site/src/content/docs/ru/guides/sidecars.mddocs-site/src/content/docs/ru/reference/configuration/server.mddocs-site/src/content/docs/zh-cn/guides/sidecars.mddocs-site/src/content/docs/zh-cn/reference/configuration/server.mdgui/src/i18n/ru.tsgui/src/pages/dashboard-overview-sections.tsxgui/src/pages/dashboard-shared.tsgui/tests/dashboard-contracts.test.tssrc/config.tssrc/reasoning-effort.tssrc/server/management/config-routes.tssrc/vision/describe.tssrc/vision/index.tstests/config.test.tstests/settings-stream-mode.test.tstests/vision-anthropic.test.tstests/vision-cache.test.ts
|
CodeRabbit re-review addressed in 7468d1f.
Validation: focused config/vision tests pass; full server suite 8216 passed / 1 unrelated flaky native-profile timing test (passes in isolation, untouched by this PR); GUI 585/585, docs build, typecheck, and privacy scan green. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs-site/src/content/docs/guides/sidecars.md (1)
97-98: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument the backend-specific cache identity.
All five guides state or imply that reasoning effort is part of every image-description cache key. The runtime contract includes it only for OpenAI. Anthropic ignores the OpenAI-specific field, so changing it must not invalidate Anthropic cache entries.
docs-site/src/content/docs/guides/sidecars.md#L97-L98: state that reasoning effort is included only in OpenAI cache keys.docs-site/src/content/docs/ja/guides/sidecars.md#L94-L96: state that reasoning effort is included only in OpenAI cache keys.docs-site/src/content/docs/ko/guides/sidecars.md#L95-L98: state that reasoning effort is included only in OpenAI cache keys.docs-site/src/content/docs/ru/guides/sidecars.md#L108-L111: state that reasoning effort is included only in OpenAI cache keys.docs-site/src/content/docs/zh-cn/guides/sidecars.md#L87-L89: state that reasoning effort is included only in OpenAI cache keys.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs-site/src/content/docs/guides/sidecars.md` around lines 97 - 98, Update the cache-key documentation to state that reasoning effort is included only in OpenAI image-description cache keys, not Anthropic keys. Apply this wording in docs-site/src/content/docs/guides/sidecars.md:97-98, docs-site/src/content/docs/ja/guides/sidecars.md:94-96, docs-site/src/content/docs/ko/guides/sidecars.md:95-98, docs-site/src/content/docs/ru/guides/sidecars.md:108-111, and docs-site/src/content/docs/zh-cn/guides/sidecars.md:87-89.Source: Path instructions
src/config.ts (1)
1327-1354: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-532): Insertion of Sensitive Information into Log File
Reachability: Internal · Exploitability: Moderate
Reachability path
● Entry tests/settings-stream-mode.test.ts │ ▼ ● Sink src/config.tsRedact invalid
visionSidecar.reasoningbefore logging it.At
src/config.ts:1345-1351,JSON.stringify(raw)can write arbitrary persisted content toconsole.warn. Log only the value type or a bounded redacted representation, while preserving the fallback-to-lowwarning. The same warning runs at lines 1665 and 1686.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config.ts` around lines 1327 - 1354, The warning in warnDegradedVisionReasoning must not log arbitrary persisted visionSidecar.reasoning content. Replace JSON.stringify(raw) with a type-only or bounded redacted representation while preserving the warning’s indication that the value falls back to low; apply the same sanitization to both other call sites of this warning.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/settings-stream-mode.test.ts`:
- Around line 304-309: Update the warning assertion in the settings-stream
regression test to capture the specific warning mentioning
visionSidecar.reasoning, then assert it includes the invalid fixture value in
its safe representation and the text indicating fallback to "low". Preserve the
existing assertions that reasoning remains undefined and providers survive.
- Around line 302-305: Ensure the warnSpy created around loadConfig and the
warnedAboutReasoning inspection is restored on every exit path. Wrap the test
operations in try/finally and call warnSpy.mockRestore() from the finally block,
or add equivalent afterEach cleanup, while preserving the existing warning
inspection behavior.
---
Outside diff comments:
In `@docs-site/src/content/docs/guides/sidecars.md`:
- Around line 97-98: Update the cache-key documentation to state that reasoning
effort is included only in OpenAI image-description cache keys, not Anthropic
keys. Apply this wording in docs-site/src/content/docs/guides/sidecars.md:97-98,
docs-site/src/content/docs/ja/guides/sidecars.md:94-96,
docs-site/src/content/docs/ko/guides/sidecars.md:95-98,
docs-site/src/content/docs/ru/guides/sidecars.md:108-111, and
docs-site/src/content/docs/zh-cn/guides/sidecars.md:87-89.
In `@src/config.ts`:
- Around line 1327-1354: The warning in warnDegradedVisionReasoning must not log
arbitrary persisted visionSidecar.reasoning content. Replace JSON.stringify(raw)
with a type-only or bounded redacted representation while preserving the
warning’s indication that the value falls back to low; apply the same
sanitization to both other call sites of this warning.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 094f235d-bb5a-4d5f-87b6-41b5cc0f0ee1
📒 Files selected for processing (7)
docs-site/src/content/docs/guides/sidecars.mddocs-site/src/content/docs/ja/guides/sidecars.mddocs-site/src/content/docs/ko/guides/sidecars.mddocs-site/src/content/docs/ru/guides/sidecars.mddocs-site/src/content/docs/zh-cn/guides/sidecars.mdsrc/config.tstests/settings-stream-mode.test.ts
|
CodeRabbit round 3 addressed in eb017e0.
Validation: typecheck, settings-stream-mode (18/18 with the strengthened assertions), and the 216-page docs build all pass. |
1 participant
Summary
low,medium,high,xhigh, andmaxin the Dashboard and management APIlowdefault for backward compatibilityWhy
The vision sidecar previously hard-coded
reasoning.effort: "low", even though the CLI already acceptedocx agent sidecar vision --reasoning. Users of vision models such asgpt-5.6-lunatherefore could not trade latency and cost for more detailed image analysis, and the CLI setting was silently ignored by the vision management path.Impact
OpenAI-backed vision descriptions now send the configured effort in the Responses request. Existing configurations behave exactly as before because the default remains
low. Anthropic vision retains the setting in shared configuration but ignores the OpenAI-specific wire field. Cache entries are separated by reasoning effort so changing the setting cannot reuse a description produced at a different effort.Validation
bun run typecheckbun run test— 8214 passed, 8 skipped, 0 failedcd gui && bun test tests— 583 passed, 0 failedcd gui && bun run lint:i18n && bun run lint && bun run buildcd docs-site && ASTRO_TELEMETRY_DISABLED=1 bun run build— 216 pages builtbun run privacy:scanSummary by CodeRabbit
New Features
Bug Fixes
Documentation